-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
카테고리, 태그, 템플릿 목록 로딩 처리 개선 #784
base: dev/fe
Are you sure you want to change the base?
Conversation
…plateList 훅 분리 및 MyTemplatePage 적용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 이 훅의 처음 이름은 useTemplateList
였는데요, 템플릿 목록을 조회해온다는 느낌을 조금 더 주고 싶어서 고민하다 변경하게 되었습니다. 그러나 여전히 'show'가 들어가는 것이 좋은 이름이라는 생각이 들지않네요🥲
해당 훅의 네이밍 추천받습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'use' prefix 뒤에 명사가 오는 것이 자연스럽다고 생각되는데, useTemplateListManagement
어떤가요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 월하랑 비슷한 의견이긴 합니다. form 훅을 useSubmitForm 보다는 useForm으로 많이 쓰는 느낌 아닐까요>? 그런 의미에서 기존 훅 네이밍도 괜찮다고 생각합니다 ㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사해요! 고민 끝에 useFilteredTemplateList
라고 바꿔보았습니다. 이 훅의 핵심 역할은 각 조건에 따라 필터링 된 템플릿 목록을 가져오는 것이라고 생각했기 때문입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오늘 프론트 회의에서 이야기했던 방식대로, 스켈레톤 대신 이전 템플릿 목록 위로 흰색 반투명한 로딩 박스가 뜨도록 했습니다.
그 이유는 스켈레톤보다는 이전 데이터라도 보고있는 것이 사용자가 덜 심심할 것이라고 생각했기 때문입니다!
{isTemplateListFetching && <TemplateListSectionLoading />} | ||
{!isTemplateListLoading && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(참고) isTemplateListFetching
과 isTemplateListLoading
는 서로 다른 상태입니다.
isTemplateListFetching
: 새로운 템플릿 목록을 조회해올때마다 true 가 됩니다. 예를 들어 태그를 누르면 그 태그에 해당하는 템플릿 목록을 가져오게 되고, 그동안 true 가 됩니다.isTemplateListLoading
: 처음 템플릿 목록을 조회해올 때 true 가 됩니다.
여기서 !isTemplateListLoading
를 해주지 않을 경우, 템플릿 목록을 조회하기 이전에 무조건 NewTemplateButton
컴포넌트가 보이기 때문에 위처럼 처리해주었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'use' prefix 뒤에 명사가 오는 것이 자연스럽다고 생각되는데, useTemplateListManagement
어떤가요
export const useKeyword = () => { | ||
const [keyword, handleKeywordChange] = useInput(''); | ||
const debouncedKeyword = useDebounce(keyword, 300); | ||
|
||
return { keyword, debouncedKeyword, handleKeywordChange }; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSearchKeyword 같은 네이밍은 어떨까요?
isEditMode, | ||
toggleIsEditMode, | ||
isDeleteModalOpen, | ||
toggleDeleteModal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit 모드 변경, deleteModal 오픈 같은 경우에는 해당 훅에서 한번에 묶일 필요가 없지 않을까 합니다.
아마도 handleDelete 때문에 함께 있는 것 같은데, 각각 editMode, deleteModal 훅을 분리하고 handleDelete 함수는 MyTemplatePage에 있어도 되지 않을까 합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그러면 네이밍도 selectList 관련으로만 지을 수 있지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로 선택과 삭제를 하나의 훅에서 관리하게 된 이유는 선택과 삭제가 긴밀하게 연결되어있다고 생각했기 때문입니다. 전체 선택이 가능한 이유는 다른 동작이 가능한 것이 아니라 오로지 삭제를 위한 것이기 때문에 선택은 삭제를 위한 하나의 과정이라고 생각하였습니다.
삭제 모달도 MyTemplate
에 있던 것을 TemplateDeleteSelection
로 옮긴 것 또한 위와 같은 생각때문이었습니다.
다만, 마위의 의견처럼 선택과 삭제를 유기적인 관점이 아니라 각각 독립된 역할의 관점에서 볼 수도 있을 것 같아요! 이걸 분리하는게 좋을지 그냥 이렇게 합쳐 두는 것이 더 좋을지 고민이 많이 됩니다!! 저의 이런 생각을 들어보시고 고민한 마위와 월하의 생각도 궁금하여 일단 길게 코멘트 남기게 되었습니다!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 월하랑 비슷한 의견이긴 합니다. form 훅을 useSubmitForm 보다는 useForm으로 많이 쓰는 느낌 아닐까요>? 그런 의미에서 기존 훅 네이밍도 괜찮다고 생각합니다 ㅋㅋㅋ
queryKey: [QUERY_KEY.TAG_LIST], | ||
queryFn: () => getTagList({ memberId }), | ||
throwOnError: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 throwOnError가 지워진 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSuspenseQuery
를 사용하면서 throwOnError
를 사용할 수 없게 되었습니다!
따라서 에러처리를 위해서는 서스펜스 위로 에러바운더리를 따로 넣어주어야 할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const categoryList = categoryData?.categories || []; | ||
|
||
return ( | ||
<Flex direction='column' gap='2.5rem' style={{ marginTop: '4.5rem' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flex에 style을 따로 주는 경우가 꽤 있어서 이런 경우에는 따로 styled로 해도 괜찮을 수도 있겠네요,, ㅎㅎ
const SkeletonButtonWrapper = styled.div` | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
|
||
width: 12.5rem; | ||
height: 3rem; /* CategoryButton과 동일한 높이 */ | ||
padding: 0.5rem; | ||
|
||
background: linear-gradient(90deg, #e0e0e0 25%, #c0c0c0 50%, #e0e0e0 75%); | ||
background-size: 200% 100%; | ||
border-radius: 8px; | ||
box-shadow: 1px 2px 8px #00000020; /* CategoryButton과 동일한 그림자 */ | ||
|
||
animation: skeleton-loading 1.5s infinite ease-in-out; | ||
|
||
@keyframes skeleton-loading { | ||
0% { | ||
background-position: -200% 0; | ||
} | ||
100% { | ||
background-position: 200% 0; | ||
} | ||
} | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
당장 리팩토링할 필요는 없을거 같지만, 스켈레톤 UI를 width, height만 조정할 수 있게 하면 공통 컴포넌트로 만들 수 있겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 생각이네요! 다만 현재 다른 작업의 우선순위가 더 높다고 생각하기 때문에 이 부분은 일단 인지하고 다음 리펙토링에서 고려해보도록 하겠습니다!
interface ConfirmDeleteModalProps { | ||
isDeleteModalOpen: boolean; | ||
toggleDeleteModal: () => void; | ||
handleDelete: () => void; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기 그냥 Props로 해도 되나요?!
|
||
const ConfirmDeleteModal = ({ isDeleteModalOpen, toggleDeleteModal, handleDelete }: ConfirmDeleteModalProps) => ( | ||
<Modal isOpen={isDeleteModalOpen} toggleModal={toggleDeleteModal} size='xsmall'> | ||
<Flex direction='column' justify='space-between' align='center' margin='1rem 0 0 0' gap='2rem'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 뭔가 style=margin-top과 margin 1 0 0 0 이 비슷해서 위 코멘트를 남겼었습니다 ㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컴포넌트를 분리할 때 이전 코드를 그대로 복붙해서 사용해서 그런가봅니다. 😂 현재는 Modal 내부 합성 컴포넌트를 활용하여 리펙토링하였습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인해야할 코멘트가 있어서 RC 남길게요~!
고생하셨습니다 헤인
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영이 깔끔하게 되었네요! 고생했습니다 헤인
(요즘 헤인 PR 리뷰를 많이 하는 것 같네욥 ㅋㅋ)
if (result.error && !result.isFetching) { | ||
throw result.error; | ||
} | ||
|
||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굿!
안녕하세요, 프론트 여러분! 헤인입니다.
매번 리팩토링 PR을 올릴 때 files changed가 너무 많아 저도 놀라는데요...
컴포넌트 분리와 훅 분리가 전혀 되지 않은 부분들을 분리하다보니 변경 사항이 많이 잡히게 되는 것 같습니다. 😂
저의 구현 의도와 주요 변경 사항들에 대해 상세하게 써보려고 노력하였으나 부족한 부분이 있다면 얼마든지 코멘트로 질문해주세요!
이름 변경 등 아주 사소한 피드백들도 환영입니다!!
⚡️ 관련 이슈
🪧 주요 목표
default.mov
default.mov
📍주요 변경 사항
1. 카테고리, 태그, 템플릿 목록
useSuspenseQueries
적용 해제useSuspenseQuery
를 사용하여 서스팬스 처리 해주었습니다.useQuery
를 사용하여 isLoding, isFetching 상태로 로딩 처리를 해주었습니다.2.
MyTemplatePage
컴포넌트 분리TopBanner
:-님의 템플릿 입니다.
를 보여주는 컴포넌트CategoryListSection
: 카테고리 목록을 불러와CategoryFilterMenu
를 보여주는 컴포넌트CategoryListSectionSkeleton
:CategoryListSection
이 오기 전에suspense
에서 보여주는 fallback 컴포넌트TagListSection
: 태그 목록을 불러와TagFilterMenu
를 보여주는 컴포넌트TagListSectionSkeleton
:TagListSection
이 오기 전에suspense
에서 보여주는 fallback 컴포넌트NewTemplateButton
: 템플릿 목록이 없을 경우 템플릿 작성을 유도하는 버튼 컴포넌트TemplateListSection
: 템플릿 목록 조회 결과에 따라NoSearchResults
,NewTemplateButton
,TemplateGrid
보여주는 컴포넌트TemplateListSectionLoading
: 템플릿 목록이 로딩중일 때 로딩을 나타내주는 컴포넌트TemplateDeleteSelection
: 템플릿 목록 선택 삭제를 위한선택 삭제
버튼 및삭제 확인 모달
이 있는 컴포넌트ConfirmDeleteModal
: 삭제 확인 모달 컴포넌트3.
useShowTemplateList
,useSelectAndDeleteTemplateList
훅으로 로직 분리MyTemplatePage
내부에서 사용되는 상태 및 상태 핸들러 함수들을 역할과 책임에 맞게 분리해보았습니다. ( UI와 로직이 한 파일에서 관리되고 있어, 파일이 몹시 길고 관심사 분리가 되지 않고있다는 느낌을 받았기에 아래와 같이 리팩토링해보았습니다.)useShowTemplateList
: 선택된 카테고리, 선택된 태그, 키워드, 정렬, 페이지 정보를 기반으로 보여줄 템플릿 목록을 가져오는 로직을 담고 있습니다. 따라서 선택된 카테고리, 선택된 태그, 키워드, 정렬, 페이지 정보를 가지고 있습니다.useSelectAndDeleteTemplateList
: 카테고리 목록에서 전체 삭제 및 선택 삭제와 관련된 로직을 담고있습니다. 따라서 현재 편집모드여부, 삭제 모달 열림 여부, 선택된 템플릿 목록 정보를 가지고 있습니다.🎸기타
router
에서CustomSuspense
로 모든 레이아웃을 감싸주고 있어서 Loading 이라고 뜨고 있는데요, 이게 현재 운영 버전(https://www.code-zap.com)에서도 뜨고있습니다. 이게 좋은 사용자 경험은 아니라고 생각되어,CustomSuspense
를 잘 꾸미거나 제거하는 방향으로 리팩토링을 해야 할 것 같습니다.🍗 PR 첫 리뷰 마감 기한
10/16(수) 18:00